-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: remove optionally using operating system in the manifest file name hash #552
Conversation
…name hash Signed-off-by: Godot Bian <[email protected]>
@@ -224,16 +224,15 @@ def _gather_upload_metadata( | |||
manifest: BaseAssetManifest, | |||
source_root: Path, | |||
manifest_name_suffix: str, | |||
# TODO - remove file_system_location_name after ASSET_SYNC_JOB_USER_FEATURE completion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a tracking ticket so we don't forget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do, it's linked in the design doc.
Quality Gate passedIssues Measures |
What was the problem/requirement? (What/Why)
attachment download
andattachment upload
commands support upload and download in a batch. They use the premisemanifest file name contains the hash of the root path where the manifest was generated for
to map manifest file to its corresponding root.However,
file_system_location_name
is used in certain cases when passed in for some reason. This lead to the following problems at the time of downloading or uploadingfile_system_location_name
file_system_location_name
What was the solution? (How)
By tracing the code, this change to optionally use
file_system_location_name
in hash was in #10. This is only used for output manifest name. There is no risk of hash collision that override the file since output manifest is partitioned at session action level (i.e.s3://XXXXX/Deadline/Manifests/farm-XXX/queue-XXX/job-XXX/step-XXX/task-XXX-0/2024-12-05T00:59:46.766370Z_sessionaction-313e8e3e1c67466684d28e2f8be07a38-1/08d256ef06279328acd95ac6a3d4f984_output
, which has one and only one operating system. Output download discovers the manifests using formatted s3 partition.Input manifest should also be unique to its root. Additionally, there is no hash check on the file name, which guarantees this change does not introduce regression.
Input manifest is already persisted as part of job
What is the impact of this change?
Output manifest file would not have
file_system_location_name
in the hash par of job name. This is to support the new approach for attachment commands.How was this change tested?
28 passed, 2 skipped, 5 warnings in 74.34s (0:01:14)
download
orasset_sync
modules? If so, then it is highly recommendedthat you ensure that the docker-based unit tests pass. [Y]
32 passed, 12 skipped in 14.59s
Was this change documented?
Does this PR introduce new dependencies?
Is this a breaking change?
This PR changes the way manifest file name is generated to be the hash of root path.
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.